-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: JSON-RPC responses async iterable iterator #1937
feat: JSON-RPC responses async iterable iterator #1937
Conversation
c31725a
to
1550216
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
while (true) { | ||
if (!state.chains.has(chainId)) | ||
throw new AlreadyDestroyedError(); | ||
if (options.disableJsonRpc) | ||
return Promise.reject(new JsonRpcDisabledError()); | ||
if (state.instance.status === "destroyed") | ||
throw state.instance.error; | ||
if (state.instance.status !== "ready") | ||
throw new Error(); // Internal error. Never supposed to happen. | ||
|
||
// Try to pop a message from the queue. | ||
const message = state.instance.instance.peekJsonRpcResponse(chainId); | ||
if (message) | ||
return message; | ||
const result = await newChain.jsonRpcResponses.next(); | ||
|
||
// If no message is available, wait for one to be. | ||
await new Promise<void>((resolve) => { | ||
state.chains.get(chainId)!.jsonRpcResponsesPromises.push(resolve) | ||
}); | ||
if (result.done) { | ||
throw new AlreadyDestroyedError(); | ||
} | ||
|
||
return result.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually much prefer if this function was left as it is, and jsonRpcResponses
was implemented on top of it. This function is much more simple than jsonRpcResponses
from an implementation point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having nextJsonRpcResponse
be an implementation of jsonRpcResponses
allow jsonRpcResponses
to be more generic here, specifically stopping the iteration without having to throw an error (by returning { done: true }
), whilst nextJsonRpc
can still throw an error if there are no more responses (the chain is already removed), keeping backward compatibility.
But in saying that, that is the only benefit that I can think of though, I don't have too much of a strong opinion on this, let me know if you think otherwise and I'll revert this right away 🙏
} | ||
}, | ||
return: async () => { | ||
newChain.remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the chain here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this behaviour make sense according to the documentation here. But I have removed it anyway, let me know what is your final judgement on this, thank you.
* @throws {@link JsonRpcDisabledError} If the JSON-RPC system was disabled in the options of the chain. | ||
* @throws {@link CrashError} If the background client has crashed. | ||
*/ | ||
jsonRpcResponses: AsyncIterableIterator<string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about?
jsonRpcResponses: AsyncIterableIterator<string> | |
readonly jsonRpcResponses: AsyncIterableIterator<string> |
And turn jsonRcpResponses
into a getter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made this a readonly
property. I think a property make more sense compared to a getter, because there's in fact only 1 iterable here:
- Each iteration yield the next value
- If multiple iterables are created and are iterated on, they will actually overlap (similar to call
nextJsonResponse
simultaneously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant by getter is to do get jsonRpcResponses() { return { next: ..., ... } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, here I believe just a TypeScript readonly
modifier is sufficient, like technically speaking other properties/methods like sendJsonRpc
, nextJsonRpcResponse
, etc are actually mutable as well. If you want to enforce immutability at runtime on the JS side, I think calling Object.freeze
on the entire chain
object is more effective.
1550216
to
e3e26e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, could you also add an entry to wasm-node/javascript/CHANGELOG.md? 🙏
e3e26e4
to
3a048a2
Compare
I've added an entry to the changelog, the file you reference doesn't seem to exist though, so I have created one accordingly 🙏 |
It was in fact |
3a048a2
to
6dc8629
Compare
Okay, thanks for clarifying, have added the entry to the correct file 💪 |
Add support for the
for await...of
statement for handling responses.Currently, to listen to all responses, you would use a
while
loop like this:This PR introduces a more concise syntax:
In addition to the syntactic improvement and avoiding the
while
loop, this approach allows for closing the stream without requiring an error to be thrown.